Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(worker): digest by key #7569

Merged
merged 1 commit into from
Jan 23, 2025

Conversation

djabarovgeorge
Copy link
Contributor

What changed? Why was the change needed?

Screenshots

Expand for optional sections

Related enterprise PR

Special notes for your reviewer

Copy link

linear bot commented Jan 23, 2025

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

revert the removal of digest key in Dashboard.

@@ -252,18 +253,22 @@ export class JobRepository extends BaseRepository<JobDBModel, JobEntity, Enforce
};
}

private buildDigestQuery(digestKey: string | undefined, digestValue: string | number | undefined) {
const digestQueryV1 = digestKey ? { [`payload.${digestKey}`]: digestValue } : null;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In v1, we handled digest aggregation by:

  1. Extracting a value from the job payload using a specified digest key
  2. Checking if another job already existed with the same digest key-value pair
  3. If a match was found, we would merge the new job with the existing one, otherwise create new one.

private buildDigestQuery(digestKey: string | undefined, digestValue: string | number | undefined) {
const digestQueryV1 = digestKey ? { [`payload.${digestKey}`]: digestValue } : null;
// Digest key parsing is handled by the framework, leaving only the digest value available here
const digestQueryV2 = !digestKey && digestValue ? { [`digest.digestValue`]: digestValue } : null;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

now we store the final digest value directly. We don't track how this value was generated - we simply check if this exact digest value already exists in our system.

Copy link

netlify bot commented Jan 23, 2025

Deploy Preview for dev-web-novu ready!

Name Link
🔨 Latest commit 428c279
🔍 Latest deploy log https://app.netlify.com/sites/dev-web-novu/deploys/67923255ef7e510007f9e426
😎 Deploy Preview https://deploy-preview-7569.dashboard.novu-staging.co
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

netlify bot commented Jan 23, 2025

Deploy Preview for dashboard-v2-novu-staging ready!

Name Link
🔨 Latest commit 428c279
🔍 Latest deploy log https://app.netlify.com/sites/dashboard-v2-novu-staging/deploys/67923255d1035f000895148c
😎 Deploy Preview https://deploy-preview-7569.dashboard-v2.novu-staging.co
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@@ -278,7 +280,7 @@ export class AddJob {
{
$set: {
'digest.type': metadata.type,
'digest.digestKey': metadata.digestKey,
'digest.digestValue': metadata.digestValue,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any backwards compatability concerns?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nvm, saw it later in the query builder

Copy link
Contributor Author

@djabarovgeorge djabarovgeorge Jan 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah this part is about the framework digest key, which was not working before.
and in the query builder we created a clear separation of the 2 versions, so it would be clear there as well.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm shocked 🤯

return `${resource}:digestKey:${digestKey}:digestValue:${digestValue}`;
}

if (digestValue) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the scenario where we have a key but not a digestValue? Is the key only the fixed subscriberId?

Copy link
Contributor Author

@djabarovgeorge djabarovgeorge Jan 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sorry i missed this one before merging.
in v2 in the end there is no digestKey only parsed key which is the digestValue, so if digestValue is missing we will use dynamic subscriberId

@djabarovgeorge djabarovgeorge merged commit 692100c into next Jan 23, 2025
49 of 50 checks passed
@djabarovgeorge djabarovgeorge deleted the nv-5240-fix-digest-key-aggregation-issue branch January 23, 2025 13:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants